-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: Update GTF_GLOB_REF after last-use copy omission #81430
Conversation
Morph is responsible for marking accesses of address-exposed locals with GTF_GLOB_REF, however the last-use copy elision can newly address expose a preexisting local. This means previous uses that morph already saw have not had GTF_GLOB_REF added. This updates the JIT to at least add GTF_GLOB_REF on locals within the same statement as the call. This is not a complete fix as nothing prevents downstream phases from reordering the call with uses from earlier statements, but this at least fixes the one issue we know of right now (which is itself stress-only). A more complete fix will likely entail moving the identifying of last-use candidates earlier and address exposing them at that point. However, that first requires ABI determination to be moved earlier, so this depends on dotnet#76926, which I expect to get to within the near future. Fix dotnet#81049
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsMorph is responsible for marking accesses of address-exposed locals with GTF_GLOB_REF, however the last-use copy elision can newly address expose a preexisting local. This means previous uses that morph already saw have not had GTF_GLOB_REF added. This updates the JIT to at least add GTF_GLOB_REF on locals within the same statement as the call. This is not a complete fix as nothing prevents downstream phases from reordering the call with uses from earlier statements, but this at least fixes the one issue we know of right now (which is itself stress-only). Note that the previous uses are not actually 'global' uses unless they actually end up getting reordered with the call. A more complete fix will likely entail moving the identifying of last-use candidates earlier and address exposing them at that point. However, that first requires ABI determination to be moved earlier, so this depends on #76926, which I expect to get to within the near future. So this is sort of an interim fix. Fix #81049
|
The current statement might not be a valid tree during morphing of a leaf node, so remark the global uses after we are done morphing instead.
cc @dotnet/jit-contrib PTAL @AndyAyersMS I'm not so happy with this fix, but I think as an interim fix until I get to #76926 it should be ok. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable interim fix.
Morph is responsible for marking accesses of address-exposed locals with GTF_GLOB_REF, however the last-use copy elision can newly address expose a preexisting local. This means previous uses that morph already saw have not had GTF_GLOB_REF added. This updates the JIT to at least add GTF_GLOB_REF on locals within the same statement as the call. This is not a complete fix as nothing prevents downstream phases from reordering the call with uses from earlier statements, but this at least fixes the one issue we know of right now (which is itself stress-only).
Note that the previous uses are not actually 'global' uses unless they actually end up getting reordered with the call.
A more complete fix will likely entail moving the identifying of last-use candidates earlier and address exposing them at that point. However, that first requires ABI determination to be moved earlier, so this depends on #76926, which I expect to get to within the near future. So this is sort of an interim fix.
Fix #81049